Make active_work match records_per_batch #1316

andyleiserson · 2024-09-26T05:15:29Z

Adjust active_work to match records_per_batch. If it is less, we will stall, since every record in the batch remains incomplete until the batch is validated. More might be okay, but making it the same seems simpler for now.

This change also modifies an existing DZKP proptest to be more extensive. I believe the test is capable of exposing both the hang when active_work is less than batch size (when run without the fix in this PR), and the hang due to read_size not dividing active work (not yet fixed).

I moved a couple tests that are doing DZKPs with various BA types from secret_sharing::vector::impls to the DZKP module. My intent was to adapt them to cover the interaction with record size, but I haven't done that yet. (The move is in a separate commit, although in the current state I don't think there's interesting diff on top of the move anyways.)

Record size tests
Tune proptest runtime for CI

akoshelev

lgtm, but it seems some tests are broken.

akoshelev · 2024-09-26T06:49:58Z

ipa-core/src/protocol/context/dzkp_malicious.rs

 }

 impl<'a> DZKPUpgraded<'a> {
    pub(super) fn new(
        validator_inner: &Arc<MaliciousDZKPValidatorInner<'a>>,
        base_ctx: MaliciousContext<'a>,
    ) -> Self {
+        // Adjust active_work to be at least records_per_batch. If it is less, we will


we should probably log the fact that we've adjusted it - maybe at the debug level

akoshelev · 2024-09-26T06:50:47Z

ipa-core/src/protocol/context/dzkp_validator.rs

+
+    #[tokio::test]
+    async fn select_semi_honest() {
+        test_select_semi_honest::<BA8>().await;


is it worth testing it for for weird types like BA3 and BA7 as well?

Yes, I think so. I had BA4 but took it out because it's not boolean_vector! (not for any good reason other than we haven't needed it). But I think it is worth having a less-than-one-byte case, and maybe even adding a new BA type so we can cover the between-one-and-two-bytes case.

akoshelev · 2024-09-26T06:53:12Z

ipa-core/src/protocol/context/dzkp_validator.rs

-            (1usize<<log_count, 1usize<<log_multiplication_amount)
-        }
+    fn record_count_strategy() -> impl Strategy<Value = usize> {
+        prop_oneof![1usize..=512, (1usize..=9).prop_map(|i| 1usize << i)]


I am being stupid and it is 12pm now - is it really testing $$2^{512}$$ records?

What it does is:

50% of the time it picks a random power of two between 2 and 512 (9 possible values)

50% of the time it picks a random integer between 1 and 512 (512 possible values)

Two reasons to focus on powers of two, although I didn't put a huge amount of thought into this (e.g. it just occurred to me to add $2^0$):

Because we plan to restrict batch sizes to powers of two.

As the count increases, it makes sense to me to sample the space more sparsely, there's relatively less interesting about testing batch sizes 399, 400, and 401 than about testing batch sizes 7, 8, and 9.

I think it is clearer to have the two prop_oneof cases on different lines, but rustfmt insisted on a single line.

akoshelev · 2024-09-26T06:54:25Z

ipa-core/src/protocol/context/malicious.rs

+        // Adjust active_work to be at least records_per_batch. The MAC validator
+        // currently configures the batcher with records_per_batch = active_work, which
+        // makes this adjustment a no-op, but we do it this way to match the DZKP validator.
+        let records_per_batch = batch.lock().unwrap().records_per_batch();


would it be better to assert this to make sure we don't miss the misalignment between MAC and ZKP?

Sure, I'm fine with that.

andyleiserson · 2024-09-26T17:46:40Z

ipa-core/src/protocol/context/validator.rs

@@ -217,7 +217,7 @@ impl<'a, F: ExtendableField> BatchValidator<'a, F> {

        // TODO: Right now we set the batch work to be equal to active_work,
        // but it does not need to be. We can make this configurable if needed.
-        let records_per_batch = ctx.active_work().get().min(total_records.get());
+        let records_per_batch = ctx.active_work().get();


Reducing if larger than total_records may have been necessary with an earlier version of the batcher, but the current version should take care of that internally, so I removed it here. No relation to the rest of these changes though.

codecov · 2024-09-26T23:04:54Z

Codecov Report

Attention: Patch coverage is 98.48485% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.52%. Comparing base (1521f8b) to head (d2512f1).
Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
ipa-core/src/protocol/context/dzkp_malicious.rs	80.00%	2 Missing ⚠️
ipa-core/src/protocol/context/malicious.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1316      +/-   ##
==========================================
+ Coverage   93.45%   93.52%   +0.07%     
==========================================
  Files         207      208       +1     
  Lines       33451    33802     +351     
==========================================
+ Hits        31260    31613     +353     
+ Misses       2191     2189       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

akoshelev · 2024-09-27T00:26:19Z

ipa-core/src/protocol/context/dzkp_validator.rs

+        for<'a> Replicated<V>: BooleanArrayMul<DZKPUpgradedSemiHonestContext<'a, NotSharded>>,
+        Standard: Distribution<V>,
+    {
+        let world = TestWorld::default();


I just realized that we probably want to use the same seed for rng and test world (it is supported via TestWorldConfig struct). That way we can make it reproducible if it ever fails

I made an issue for this. It's definitely worth doing in general, but it doesn't seem all that important for this particular test case, where the input values should be unrelated to the behavior of the test.

andyleiserson · 2024-09-27T04:26:04Z

The newest commit is to fix the failure in https://github.com/andyleiserson/ipa/actions/runs/11061292252/job/30733595263.

andyleiserson · 2024-09-27T16:42:01Z

Draft runs:

100k: https://draft-mpc.vercel.app/query/view/agile-book2024-09-27T1606, successful
2M: https://draft-mpc.vercel.app/query/view/aided-icon2024-09-27T1612, stalled

I suspect the issue is that, without the changes to make batch sizes power of two / align read size with batch sizes, we can't remove the change that uses active work as the batch size for attribution.

andyleiserson · 2024-09-27T17:51:56Z

Draft result for 2M records at d2512f1: https://draft-mpc.vercel.app/query/view/drunk-widow2024-09-27T1647

It failed due to #1308. But when I added the workaround (that was 2333492), CI failed due to excessive memory allocation. The 2M draft run at 2333492 passed: https://draft-mpc.vercel.app/query/view/iron-input2024-09-27T1735.

akoshelev · 2024-09-27T18:38:51Z

CI failed due to excessive memory allocation.
Can you please post a link? It's first time we are failing on OOM and I want to quickly check whether we're missing something

andyleiserson · 2024-09-27T18:47:26Z

Here's the failure with large batch size for breakdown reveal aggregation: https://github.com/private-attribution/ipa/actions/runs/11075152193/job/30775478393

andyleiserson added 3 commits September 25, 2024 21:02

Move some tests

b628bf2

Make the DZKP batching proptest more comprehensive

a4c6f03

Make active_work at least records_per_batch

1a6e388

akoshelev approved these changes Sep 26, 2024

View reviewed changes

andyleiserson commented Sep 26, 2024

View reviewed changes

akoshelev mentioned this pull request Sep 26, 2024

Fix send buffer misalignment issue #1307

Closed

andyleiserson added 2 commits September 26, 2024 15:32

Improvements to batching proptest

706dcbe

Revise based on PR feedback and offline discussion

c5c0ccf

akoshelev reviewed Sep 27, 2024

View reviewed changes

Fix a bug in the batcher

966160f

private-attribution deleted a comment from TeONy46 Sep 27, 2024

andyleiserson mentioned this pull request Sep 27, 2024

Ensure TestWorld seed is propagated to all RNGs #1321

Closed

andyleiserson changed the title ~~Make active_work at least records_per_batch~~ Make active_work match records_per_batch Sep 27, 2024

Restore the batch size kludge for attribution

d2512f1

andyleiserson force-pushed the active-work branch from 2333492 to d2512f1 Compare September 27, 2024 17:46

andyleiserson merged commit 59ca3e7 into private-attribution:main Sep 27, 2024
22 checks passed

andyleiserson deleted the active-work branch September 27, 2024 18:33

akoshelev mentioned this pull request Oct 2, 2024

Fix send buffer misalignment [No 2] #1332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make active_work match records_per_batch #1316

Make active_work match records_per_batch #1316

andyleiserson commented Sep 26, 2024 •

edited

Loading

akoshelev left a comment

akoshelev Sep 26, 2024

akoshelev Sep 26, 2024

andyleiserson Sep 26, 2024

akoshelev Sep 26, 2024

andyleiserson Sep 26, 2024

akoshelev Sep 26, 2024

andyleiserson Sep 26, 2024

andyleiserson Sep 26, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

akoshelev Sep 27, 2024

andyleiserson Sep 27, 2024

andyleiserson commented Sep 27, 2024

andyleiserson commented Sep 27, 2024

andyleiserson commented Sep 27, 2024 •

edited

Loading

akoshelev commented Sep 27, 2024

andyleiserson commented Sep 27, 2024

Make active_work match records_per_batch #1316

Make active_work match records_per_batch #1316

Conversation

andyleiserson commented Sep 26, 2024 • edited Loading

akoshelev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleiserson commented Sep 27, 2024

andyleiserson commented Sep 27, 2024

andyleiserson commented Sep 27, 2024 • edited Loading

akoshelev commented Sep 27, 2024

andyleiserson commented Sep 27, 2024

andyleiserson commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

andyleiserson commented Sep 27, 2024 •

edited

Loading